Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AMP-101866 textarea serialization #1552

Conversation

lewgordon-amplitude
Copy link

@lewgordon-amplitude lewgordon-amplitude commented Aug 1, 2024

No description provided.

Kelly Wallach and others added 30 commits October 18, 2023 14:50
perf: only call createHTMLDocument when it is needed
feat: skip through inactive periods instead of fast forward
fix(rrweb-snapshot): pass maskInputFn correctly
eoghanmurray and others added 26 commits June 21, 2024 10:01
… a url - need to consume quotes.

Thanks [email protected] for reporting and isolating this case
…ng populated if there were no hover selectors on the stylesheet

 - not committing the test, but modifying the existing 'add a hover class to a previously processed css string' as follows shows the problem:

--- a/packages/rrweb-snapshot/test/rebuild.test.ts
+++ b/packages/rrweb-snapshot/test/rebuild.test.ts
@@ -151,6 +185,7 @@ describe('rebuild', function () {
         path.resolve(__dirname, './css/benchmark.css'),
         'utf8',
       );
+      cssText = cssText.replace(/:hover/g, '');

       const start = process.hrtime();
       addHoverClass(cssText, cache);
… looks out at the browser viewport whereas we need it to look at the replayer iframe viewport
…or lists. I believe these were failing in a previous version of rrweb as I had some local patches that no longer seem to be needed to handle these cases
…n just :hover. I believe this function is only exported for the purposes of use in the tests
fix(rrweb): Fix some css issues with :hover and rewrite max-device-width
* Fix serialization and mutation of <textarea> elements taking account the duality that the value can be set in either the child node, or in the value _parameter_ (not attribute)

* Backwards compatibility: Bug fix and regression test for rrweb-io#112
 - this is to fix up 'historical' recordings, as duplicate textarea content should no longer be being created at record time
 - new test shows what the snapshot generated by previous versions of rrweb used to look like, hence 'bad'
 - original 0efe23f fix either didn't work or no longer works due to childNodes being appended subsequent to this part of the code
 - during review, we also verified that the `_cssText` case should still be handled okay, as there's currently no scenario where csstext is present with css child nodes of a <style>

* Masking: Fix that textarea values were being missed by the masking system if the value was recorded as a child node
 - I didn't notice that form.html was used in other tests, so lucky that I noticed that those tests also should have the 'pre value' masked out

* Simplify by always storing the textarea value in the `.value` attribute (from it's DOM property) and not as a childNode. It should still be rebuilt as a childNode rather than a property
---------

Authored-by: eoghanmurray <[email protected]>
Copy link

changeset-bot bot commented Aug 1, 2024

🦋 Changeset detected

Latest commit: 8cb959c

The changes in this PR will be included in the next version bump.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lewgordon-amplitude lewgordon-amplitude deleted the AMP-101866-textarea-serialization branch August 1, 2024 15:02
@lewgordon-amplitude lewgordon-amplitude restored the AMP-101866-textarea-serialization branch August 1, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants